-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add component wrapper helper to textarea #4574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me, but the Percy diff implies something is up. Can you confirm if the Percy diff is what you expect?
@@ -102,7 +102,7 @@ examples: | |||
<%= component %> | |||
<% end %> | |||
data: | |||
id: "contextual-guidance-id" | |||
textarea_id: "contextual-guidance-id" | |||
label: | |||
text: "Title" | |||
bold: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs the uses_component_wrapper_helper: true
line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, have added. Looking into the visual diff now 👍
6c9a01e
to
8be3d17
Compare
8be3d17
to
74d73f7
Compare
74d73f7
to
9441b86
Compare
2ac455e
to
fc77102
Compare
99cc33b
to
a96a8d0
Compare
a96a8d0
to
a43724a
Compare
@AshGDS finally fixed the visual diff - there was a surprisingly complex thing happening with the textarea and the elements that followed it, but I think I've finally got it 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
- breaking change, renames `id` option to `textarea_id` as this is applied to a child element and `id` is applied to the parent by the component wrapper - removes the shared helper, instead relying on the component wrapper helper to provide the same functionality - removes a bit of code that sets the margin back to normal if an invalid value is passed, but this outcome is now handled by the component wrapper
- also improve tests, remove redundant data attributes test (component doesn't accept data attributes)
a43724a
to
876fc91
Compare
* Remove margin top from search component ([PR #4581](#4581)) * **BREAKING** Add component wrapper helper to textarea ([PR #4574](#4574)) * Remove margin top from print link ([PR #4577](#4577)) * **BREAKING** Use component wrapper on print link component ([PR #4576](#4576)) * Refactor single page notification component ([PR #4501](#4501)) * Remove shared helper from button component ([PR #4569](#4569)) * Remove shared helper from inset text component ([PR #4571](#4571)) * Use component wrapper on contextual footer ([PR #4562](#4562)) * Update Govspeak "Warning Text" component styles ([PR #4487](#4487)) * Make "Add another" component styles more specific ([PR #4579](#4579)) * Translate "and" connective in metadata component to Chinese, Russian and Arabic ([PR #4580](#4580))
What
Apply the component wrapper helper to the textarea component.
id
option totextarea_id
as this is applied to a child element andid
is applied to the parent by the component wrappermargin_bottom
To deal with this breaking change, other PRs will need to be raised in applications as follows ahead of deployment.
id
option collections-publisher#2373id
id
This change also impacts the character count component, which wraps the textarea component. The character count component accepts an
id
option, which is then passed to the textarea to be the id of the textarea element (not the component id). I've updated the component to explicitly pass this when it exists, because there was a corner case where passing an id as part of the textarea object overwrote the value of id passed directly to character count. Hopefully the tests clarify this behaviour (without causing a breaking change to the character count component).Why
The code for
margin_bottom
is moving from the shared helper to the component wrapper helper, so we need to remove any instance of this before the work can be completed. Additionally, having the component wrapper on a component helps reduce code and maintenance effort.Visual Changes
None.
Trello card: https://trello.com/c/GQ1p2oSC/438-not-doing-add-component-wrapper-helper-to-form-textarea-component